feat(aws-codepipeline): make input and output artifact names optional when creating Actions by skinny85 · Pull Request #845 · aws/aws-cdk

skinny85 · 2018-10-03T23:49:20Z

Previously, we always required customers to explicitly name the output artifacts the Actions used in the Pipeline, and to explicitly "wire together" the outputs of one Action as inputs to another. With this change, the CodePipeline Construct generates artifact names, if the customer didn't provide one explicitly, and tries to find the first available output artifact to use as input to a newly created Action that needs it, thus turning both the input and output artifacts from required to optional properties.

@eladb @RomainMuller I also had one more suggestion. I've always hated the artifactName property name of the Actions. It's misleading, and inconsistent with inputArtifact. How would you guys feel about a breaking change to rename that property to outputArtifactName?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

RomainMuller · 2018-10-04T09:18:37Z

I'm in favor of changing to outputArtifactName. Being said, I've also been a bit startled by the CodeBuild action having .artifact as well as .outputArtifacts and not being clear on whether they overlap or not (aka whether .artifact is the first entry of .outputArtifacts or not).

RomainMuller · 2018-10-04T09:20:42Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

   * @returns the newly created {@link PipelineBuildAction} build Action
   */
-  public addBuildToPipeline(stage: codepipeline.IStage, name: string, props: CommonPipelineBuildActionProps): PipelineBuildAction {
+  public addBuildToPipeline(stage: codepipeline.IStage, name: string, props?: CommonPipelineBuildActionProps): PipelineBuildAction {


Default props to {}.

RomainMuller · 2018-10-04T09:21:18Z

packages/@aws-cdk/aws-codecommit/lib/repository.ts

   * @returns the newly created {@link PipelineSourceAction}
   */
-  public addToPipeline(stage: actions.IStage, name: string, props: CommonPipelineSourceActionProps): PipelineSourceAction {
+  public addToPipeline(stage: actions.IStage, name: string, props?: CommonPipelineSourceActionProps): PipelineSourceAction {


Default props to {}.

RomainMuller · 2018-10-04T09:25:50Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts


-  protected addOutputArtifact(name: string): Artifact {
-    const artifact = new Artifact(this, name);
+  protected addOutputArtifact(name?: string): Artifact {


You can instead do:

protected addOutputArtifact(name: string = this.stage._generateOutputArtifactName(this)) { const artifact = new Artifact(this, outputArtifactName); retenu artifact; }

I don't think that will work. I want to call this method from the Action subclasses with props.outputArtifactName, where outputArtifactName is an optional string.

I think @RomainMuller is right, you should be able to do that. If you pass undefined to name (which will be the case if props.outputArtifactName is not defined, it will apply the default.

I stand corrected :) will change.

RomainMuller · 2018-10-04T09:26:36Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts


-  protected addInputArtifact(artifact: Artifact): Action {
-    this._inputArtifacts.push(artifact);
+  protected addInputArtifact(artifact?: Artifact): Action {


Similar sugaring tip as before applies there.

Same reply as above (I'm actually using the optional parameter).

RomainMuller · 2018-10-04T09:30:04Z

packages/@aws-cdk/aws-codepipeline/lib/stage.ts

  }

+  public _generateOutputArtifactName(action: actions.Action): string {
+    return (this.pipeline as any)._generateOutputArtifactName(this, action);


Honestly I don't like having to do this. I'd rather actually make the methods public & mark them s internal-use-only, which is already semi-obvious from their _-prepended names.

Does it make much difference though?

Main difference is the compiler stops trying to help the moment you've done as any, so you might miss breaking changes if you rename stuff & there is no test coverage through there.

RomainMuller · 2018-10-04T09:31:58Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

+  private _generateOutputArtifactName(stage: actions.IStage, action: actions.Action): string {
+    // for now, just return a generic output artifact,
+    // ignoring the names of both the Stage, and the Action
+    return 'Artifact_' + (++this.artifactsCounter);


The drawback is that this makes artifact names dependent on generation order. This means a user refactoring code & trying to ensure they didn't inadvertently change their infrastructure cannot rely on a simple diff test.

I would like this to be order-independent unless we have a very good reason why it's better otherwise (I understand this is a simplicity vs. user-friendliness tradeoff).

I see. What would you see as the artifact name then? Some combination of Stage & Action names? `Artifact_${stage.name}_${action.name}`?

We can use the uniqueId of the construct.

One disadvantage of this: the artifact names are now super ugly (Artifact_awscdkcodepipelinecodecommitcodebuildMyBuildProjectbuild61B48DC4).

But does anyone care?

RomainMuller · 2018-10-04T09:40:12Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

+
+  // ignore unused private method (it's actually used in Stage)
+  // @ts-ignore
+  private _findInputArtifactFor(stage: actions.IStage, action: actions.Action): actions.Artifact {


The defaulting behavior that involves this particular element has a caveat that changing the runOrder after the defaulting behavior has happened will cause incorrect values to be used... A solutions is to make the runOder immutable & set at construction time?

Yep. Not sure if it's a huge deal, as you can always name the artifacts directly, but I'm fine with making runOrder immutable (the rest of the properties of Actions already are).

skinny85 · 2018-10-04T16:49:13Z

I'm in favor of changing to outputArtifactName. Being said, I've also been a bit startled by the CodeBuild action having .artifact as well as .outputArtifacts and not being clear on whether they overlap or not (aka whether .artifact is the first entry of .outputArtifacts or not).

Actually, that's a great point. The issue is that .outputArtifacts is an 'internal' thing (customers are not supposed to call it), while .artifact is not (you should call it, when wiring the Actions together). Unfortunately, we don't have a great mechanism for differentiating these - we could conceivably change .outputArtifacts to ._outputArtifacts... But I agree 100% that .artifact should be called .outputArtifact.

eladb · 2018-10-04T21:42:59Z

packages/@aws-cdk/aws-codebuild/lib/pipeline-actions.ts

+   *
+   * @default an auto-generated name will be used
   */
  artifactName?: string;


+1 on changing this to outputArtifactName

Done (everywhere).

eladb · 2018-10-04T21:43:17Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

   */
-  public addBuildToPipeline(stage: codepipeline.IStage, name: string, props: CommonPipelineBuildActionProps): PipelineBuildAction {
+  public addBuildToPipeline(stage: codepipeline.IStage, name: string, props?: CommonPipelineBuildActionProps): PipelineBuildAction {
    return new PipelineBuildAction(this.parent!, name, {


change this.parent! to this please

eladb · 2018-10-04T21:43:35Z

packages/@aws-cdk/aws-codecommit/lib/repository.ts

   */
-  public addToPipeline(stage: actions.IStage, name: string, props: CommonPipelineSourceActionProps): PipelineSourceAction {
+  public addToPipeline(stage: actions.IStage, name: string, props?: CommonPipelineSourceActionProps): PipelineSourceAction {
    return new PipelineSourceAction(this.parent!, name, {


this.parent! => this

eladb · 2018-10-04T21:45:54Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+   *
+   * @param action the Action to generate the output artifact name for
+   */
+  _generateOutputArtifactName(action: Action): string;


I wonder if it would be more elegant to hide all these internal methods under some _internal scope:

interface IStageInternal { attachAction(...); generateOutputArtifactName(...); findInputArtifactFor(...); }; interface IStage { // public API // internal API _internal: IStageInternal; }

eladb · 2018-10-04T21:48:30Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts


-  protected addOutputArtifact(name: string): Artifact {
-    const artifact = new Artifact(this, name);
+  protected addOutputArtifact(name?: string): Artifact {


I think @RomainMuller is right, you should be able to do that. If you pass undefined to name (which will be the case if props.outputArtifactName is not defined, it will apply the default.

eladb · 2018-10-04T21:50:32Z

packages/@aws-cdk/aws-codepipeline-api/lib/build-action.ts

-    if (props.artifactName) {
-      this.artifact = this.addOutputArtifact(props.artifactName);
-    }
+    this.artifact = this.addOutputArtifact(props.artifactName);


This is a change in behavior. This means that now BuildAction will always have an output artifact defined for it. Could be totally fine, but what happens if the CodeBuild project doesn't specify any artifacts in buildspec.yaml but you still define an output artifact?

Good call out. I'll check what happens.

Confirmed it doesn't cause any problems - there simply is no output artifact produced from the CodeBuild Project in this case, but the build Action itself succeeds without issues.

eladb · 2018-10-04T21:52:21Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

+  private _generateOutputArtifactName(stage: actions.IStage, action: actions.Action): string {
+    // for now, just return a generic output artifact,
+    // ignoring the names of both the Stage, and the Action
+    return 'Artifact_' + (++this.artifactsCounter);


We can use the uniqueId of the construct.

eladb · 2018-10-04T21:53:05Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

+
+  // ignore unused private method (it's actually used in Stage)
+  // @ts-ignore
+  private _findInputArtifactFor(stage: actions.IStage, action: actions.Action): actions.Artifact {


add doc that describes what this method is doing

eladb · 2018-10-04T21:55:23Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+   *
+   * @param action the Action to find the input artifact for
+   */
+  _findInputArtifactFor(action: Action): Artifact;


Rename to findInputArtifact

eladb · 2018-10-04T21:55:33Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+   * Finds an input artifact for the given Action
+   * among the existing output artifacts currently in the Pipeline.
+   * This is an internal operation -
+   * you should never need to call it directly.


Provide some details on the algorithm

Done (it's not an easy one to describe...).

skinny85 · 2018-10-05T01:06:20Z

Updated with review comments.

There are 2 more things that could be nice to change while we're in this area:

The action.outputArtifacts property that @RomainMuller mentioned. Now we have action.outputArtifacts and action.outputArtifact, which is pretty confusing. I think it might be a good time to rename outputArtifacts to _outputArtifacts.
I find this code very weird:

  protected addChild(child: cdk.Construct, name: string) {
    super.addChild(child, name);
    if (child instanceof Artifact) {
      this._outputArtifacts.push(child);
    }
  }

  protected addOutputArtifact(name: string = this.stage._internal._generateOutputArtifactName(this)): Artifact {
    const artifact = new Artifact(this, name);
    return artifact;
  }

It kind of abuses the Construct hierarchy, and I have no idea what these extra artifacts would accomplish - I think Action should only take artifacts as arguments when constructing them. I would like to remove this code.

@RomainMuller @eladb thoughts on these 2 points?

RomainMuller · 2018-10-05T09:33:05Z

Regarding the outputArtifact and _outputArtifacts - I'm still not so clear how we deal with the case when an action produces more than one artifact (e.g. CodeBuild step with additional artifacts). I'm definitely in favor of marking the "internal-only" property with a leading _, however want to make sure we're not "losing" feature surface in doing so.

Regarding the second point, I think I'm okay dropping those if we don't have a clear use-case for them. We can always re-introduce if it turns out to be needed for something that cannot be done another way (unlikely).

eladb · 2018-10-05T14:45:57Z

@skinny85 I agree that output artifacts should be defined during initialization and we can get rid of addOutputArtifact

skinny85 · 2018-10-05T17:13:28Z

Regarding the outputArtifact and _outputArtifacts - I'm still not so clear how we deal with the case when an action produces more than one artifact (e.g. CodeBuild step with additional artifacts). I'm definitely in favor of marking the "internal-only" property with a leading _, however want to make sure we're not "losing" feature surface in doing so.

Can you elaborate on this @RomainMuller ? When can a CodeBuild project produce multiple artifacts?

skinny85 · 2018-10-08T19:40:15Z

@RomainMuller ping

According to a table in the AWS docs, CodeBuild can have a maximum of 1 output.

RomainMuller · 2018-10-10T15:10:01Z

@skinny85 well that's interesting... What happens now when your build spec specifies secondary-artifacts?

And then, this still leaves me with the question of how do I access this "one" artifact... Right now the only way I found to work is codeBuldAction.outputArtifacts[0], which looks... inelegant. And would look even more so once renamed to _outputArtifacts. There is an artifact attribute, but it is always undefined.

skinny85 · 2018-10-10T18:22:07Z

@skinny85 well that's interesting... What happens now when your build spec specifies secondary-artifacts?

I assume they're ignored by CodePipeline.

And then, this still leaves me with the question of how do I access this "one" artifact... Right now the only way I found to work is codeBuldAction.outputArtifacts[0], which looks... inelegant. And would look even more so once renamed to _outputArtifacts. There is an artifact attribute, but it is always undefined.

This is the buildAction.artifact property, renamed in this PR to buildAction.outputArtifact to make it clear what it does.

Do the changes here now make sense?

RomainMuller · 2018-10-11T07:09:25Z

Thanks for the clarification, @skinny85. I'm cool with this (and sorry the conversation dragged for so long here).

skinny85 · 2018-10-11T18:55:24Z

Rebased & made the two changes we discussed above.

… when creating Actions. BREAKING CHANGE: this commit contains the following breaking changes: * Rename 'artifactName' in Action construction properties to 'outputArtifactName' * Rename the 'artifact' property of Actions to 'outputArtifact' * No longer allow adding output artifacts to Actions by instantiating the Artifact class * Rename Action#input/outputArtifacts properties to _input/_outputArtifacts Previously, we always required customers to explicitly name the output artifacts the Actions used in the Pipeline, and to explicitly "wire together" the outputs of one Action as inputs to another. With this change, the CodePipeline Construct generates artifact names, if the customer didn't provide one explicitly, and tries to find the first available output artifact to use as input to a newly created Action that needs it, thus turning both the input and output artifacts from required to optional properties.

skinny85 · 2018-10-11T19:25:45Z

Missed a usage of the old inputArtifacts property in a CFN unit test.

skinny85 · 2018-10-11T20:05:05Z

Thanks for the clarification, @skinny85. I'm cool with this (and sorry the conversation dragged for so long here).

No worries, and thanks for the review!

__IMPORTANT NOTE__: This release includes a [breaking change](#845) in the AWS CodePipeline construct library: * The `inputArtifacts` and `outputArtifacts` properties of `Action` were intended for internal usage only, and have consequently been renamed to `_inputArtifacts` and `_outputArtifacts` respectively. * The `artifact` property of `Action` classes was renamed to `outputArtifact`. * The `artifactName` property of `Action` classes was renamed to `outputArtifactName`. * It is no longer possible to add output artifacts to `Actions` by instantiating `Artifact`. This release also includes a [fix](#911) for a bug that would make the toolkit unusable for multi-stack applications. In order to benefit from this fix, a globally installed CDK toolkit must also be updated: ```shell $ npm i -g aws-cdk $ cdk --version 0.12.0 (build ...) ``` Like always, you will also need to update your project's library versions: |Language|Update?| |--------|-------| |JavaScript/TypeScript (npm)|[`npx npm-check-updates -u`](https://www.npmjs.com/package/npm-check-updates)| |Java (maven)|[`mvn versions:use-latest-versions`](https://www.mojohaus.org/versions-maven-plugin/use-latest-versions-mojo.html) |.NET (NuGet)|[`nuget update`](https://docs.microsoft.com/en-us/nuget/tools/cli-ref-update) * **aws-cdk:** multi-stack apps can be synthesized or deployed [#911](#911). * **@aws-cdk/aws-codebuild:** allow passing oauth token to GitHubEnterpriseSource [#908](#908) * **@aws-cdk/aws-codepipeline:** make input and output artifact names optional when creating Actions. [#845](#945) * **@aws-cdk/aws-cloudformation:** add permission management to CreateUpdate and Delete Stack CodePipeline Actions. [#880](#880)

* **aws-codebuild:** allow passing oauth token to GitHubEnterpriseSource ([#908](#908)) ([c23da91](c23da91)) * **toolkit:** multi-stack apps cannot be synthesized or deployed ([#911](#911)) ([5511076](5511076)), closes [#868](#868) [#294](#294) [#910](#910) * **aws-cloudformation:** add permission management to CreateUpdate and Delete Stack CodePipeline Actions. ([#880](#880)) ([8b3ae43](8b3ae43)) * **aws-codepipeline:** make input and output artifact names optional when creating Actions. ([#845](#845)) ([3d91c93](3d91c93)) * **aws-codepipeline:** this commit contains the following breaking changes: * Rename 'artifactName' in Action construction properties to 'outputArtifactName' * Rename the 'artifact' property of Actions to 'outputArtifact' * No longer allow adding output artifacts to Actions by instantiating the Artifact class * Rename Action#input/outputArtifacts properties to _input/_outputArtifacts Previously, we always required customers to explicitly name the output artifacts the Actions used in the Pipeline, and to explicitly "wire together" the outputs of one Action as inputs to another. With this change, the CodePipeline Construct generates artifact names, if the customer didn't provide one explicitly, and tries to find the first available output artifact to use as input to a newly created Action that needs it, thus turning both the input and output artifacts from required to optional properties.

skinny85 requested review from RomainMuller and eladb October 3, 2018 23:49

RomainMuller reviewed Oct 4, 2018

View reviewed changes

eladb reviewed Oct 4, 2018

View reviewed changes

skinny85 force-pushed the feature/artifacts-api branch from b8e701c to 5cc4f3c Compare October 5, 2018 01:03

skinny85 mentioned this pull request Oct 5, 2018

Allow specifying the runOrder property when creating CodePipeline Actions #776

Merged

RomainMuller approved these changes Oct 11, 2018

View reviewed changes

skinny85 force-pushed the feature/artifacts-api branch from 5cc4f3c to 8280ca1 Compare October 11, 2018 18:54

skinny85 force-pushed the feature/artifacts-api branch from 8280ca1 to fedc18e Compare October 11, 2018 19:25

skinny85 merged commit 3d91c93 into aws:master Oct 11, 2018

skinny85 deleted the feature/artifacts-api branch October 11, 2018 20:07

RomainMuller mentioned this pull request Oct 12, 2018

v0.12.0 #912

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Conversation

skinny85 commented Oct 3, 2018

Uh oh!

RomainMuller commented Oct 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Oct 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

skinny85 commented Oct 8, 2018 •

edited

Loading